Conversation
|
✅ Build complete for PR #199. 🔗 Download the cmk binaries (expires on January 22, 2026) |
vishesh92
force-pushed
the
update-readline-and-remove-vendors
branch
from
97977d5 to
22a5e90
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR migrates from the unmaintained github.com/chzyer/readline library to github.com/ergochat/readline and removes the vendor directory dependency management approach in favor of standard Go module handling.
- Replaces
chzyer/readlinewithergochat/readlinev0.1.3 - Removes all vendor-related build flags and commands across build configuration files
- Updates dependency versions (golang.org/x/sys and adds golang.org/x/text)
Reviewed Changes
Copilot reviewed 4 out of 512 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| go.mod | Updates readline dependency and bumps indirect dependencies |
| cli/prompt.go | Changes readline import to use ergochat package |
| Makefile | Removes -mod=vendor flags from all build commands and vendor dependencies from targets |
| snap/snapcraft.yaml | Removes go mod vendor command from snap build process |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@vishesh92 there were some customisations in the existing lib, not sure they need to be included with the change. |
Let me compare with the upstream readline library. Do you know if there is any other library which was changed? We should ideally move away from vendor based setup. |
vishesh92
force-pushed
the
update-readline-and-remove-vendors
branch
from
d86f1f8 to
eceb07f
Compare
|
@shwstppr I have migrated the changes in readline package to the new one and keeping the vendors directory. |
vishesh92
changed the title
shwstppr
left a comment
shwstppr
left a comment
There was a problem hiding this comment.
@vishesh92 tested build for this PR. For some reasons prompt goes missing when autocompletion is invoked
cmk-prompt-goes-missing.webm
shwstppr
left a comment
shwstppr
left a comment
There was a problem hiding this comment.
LGTM
Not seeing any regressions due to library change
Good to have pagination when a lot of autocomplete options
There was a problem hiding this comment.
I could not find this license in the https://www.apache.org/legal/resolved.html though it seems ok.
There was a problem hiding this comment.
guess rat agrees, so we should be fine??
There was a problem hiding this comment.
This is the BSD 3-Clause license, which Apache considers acceptable (Category A)
There was a problem hiding this comment.
@vishesh92 , a question from jira, can you point to the upstream library that this file is pulled from?
vishesh92
force-pushed
the
update-readline-and-remove-vendors
branch
from
8d824e6 to
308c74a
Compare
vishesh92
force-pushed
the
update-readline-and-remove-vendors
branch
from
308c74a to
4041770
Compare
4 participants
Fixes #137 and removes vendor directory.